-
Notifications
You must be signed in to change notification settings - Fork 101
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
C#: add experimental NativeAOT-LLVM support #713
Conversation
c915a9f
to
47cd946
Compare
_console_log_imp((LogLevel){fd == STDERR_FILENO ? /*WARN*/ 1 : /*INFO*/ 2}, | ||
CSTR("wasi"), CSTR(__FILE__), __LINE__, iovs[i].buf, | ||
iovs[i].buf_len); | ||
// _console_log_imp((LogLevel){fd == STDERR_FILENO ? /*WARN*/ 1 : /*INFO*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: uncomment this back and verify it works with NAOT (I remember commenting it out because there were some issues).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(is this a thing that should happen now?)
@RReverser I just want to double-check - this change looks backwards-compatible, is that right? |
It's meant to be, yes, but it's waiting for review as I'd like someone else to double-check that no existing functionality with existing backend was accidentally broken in the process. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a few questions!
I was able to test that I can create & build a C# module on Linux (i.e. a flow without the fancy new stuff).
But I was only able to test up to the point of creating a multipart dotnet.wasm
, because I apparently don't have wasi-sdk
installed, and that seems to require a big installation rabbit hole of clang
etc. as well.
I have not tested under Windows - do you mainly want test coverage on the "old" build path, or the "new" one?
_console_log_imp((LogLevel){fd == STDERR_FILENO ? /*WARN*/ 1 : /*INFO*/ 2}, | ||
CSTR("wasi"), CSTR(__FILE__), __LINE__, iovs[i].buf, | ||
iovs[i].buf_len); | ||
// _console_log_imp((LogLevel){fd == STDERR_FILENO ? /*WARN*/ 1 : /*INFO*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(is this a thing that should happen now?)
It should download it for you behind the scenes, you don't need to install WASI SDK yourself. Are you saying it didn't? |
I only ran under Linux, but yes that seems to be the case. My build outputs were |
No, we definitely download it for Linux too - you can see conditions like It's possible some distros or architectures will need manual installation and/or building though. You are not on ARM by any chance? (like M1)
FWIW unless you are building yourself, this shouldn't be the case, WASI SDK is self-contained and has its own special version of Clang and so on. |
I'm on a pretty standard Ubuntu Linux, x86-64, etc. I do have the Maybe I'm making bad assumptions. Does this look correct/expected? Successful build,
|
Ah you need to do But, as a user, you shouldn't do this for STDB modules anyway - just use |
Hm, this is what I'm getting:
I swear I saw it work at one point last week... I'm sure I've messed something up (this is the same on |
Okay, got it successfully building & publishing without any complaints! For some reason, it works for me in the module directories under Anyway, now it works as expected! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "old" codepath works identically for me on Linux, as far as I can tell!
(Diff also LGTM, but I have no domain expertise here)
47cd946
to
af2d246
Compare
Successfully built with NativeAOT under Windows as well! |
74bb03a
to
33789f8
Compare
|
Weird, I don't know what it is , but sometimes it seems to lose track of my emscripten.. no idea why. I do sometimes get it publishing though. The publish took a very long time (a minute or two, just for the "publishing module" step). I'm not sure if that was because of the NativeAOT stuff, or some other variable. |
My |
It looks like about 45s for the upload without native AOT (database opened -> module logs start), vs ~150s with NativeAOT! (This does not include build time!) |
NativeAOT does appear faster, by roughly 50%.
My static Int64 BigFuncInner( Int64 i, int steps )
{
Log($"[{steps}] {nameof(BigFuncInner)} : {nameof(i)} = {i}");
if ( i <= 1 )
{
return i;
}
return BigFuncInner( i - 2, steps + 1 ) + BigFuncInner( i - 1, steps + 1 );
}
[SpacetimeDB.Reducer("fib")]
public static void BigFunc( DbEventArgs dbEvent, Int64 i )
{
Log($"Start {nameof(BigFunc)}({i})");
var r = BigFuncInner( i, 0 );
Log($"{nameof(BigFunc)}({i}) returned {r}");
} |
Just in case: are those timings with release version of STDB CLI or a debug one? |
Well, that's a good call. Much faster with a release build; both uploads complete in a few seconds. Also a significantly better runtime performance improvement:
|
Heh, classic :) |
I'm going to submit docs separately. |
Description of Changes
Adds alternative experimental mode for building C# modules. For now, it requires user to:
EXPERIMENTAL_WASM_AOT=1
environment variable.The development experience will be improved in the future, but for now this should be enough to get some more testing.
Initial numbers suggest it's roughly 15-20x faster in simple cases (where module language overhead dominates over DB operations).
I already published those packages as prerelease versions, so, once this PR is merged and STDB can correctly find Wasm file produced by this mode, users should be able to test via
in their
.csproj
projects.API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
3 - this is somewhat risky because, in order to add conditional compilation, I had to modify paths that affect regular compilation too. It seems to work correctly, but testing by more people would be appreciated.
How complicated do you think these changes are? Grade on a scale from 1 to 5,
where 1 is a trivial change, and 5 is a deep-reaching and complex change.
This complexity rating applies not only to the complexity apparent in the diff,
but also to its interactions with existing and future code.
If you answered more than a 2, explain what is complex about the PR,
and what other components it interacts with in potentially concerning ways.